Skip to content

edge3: Exit non-zero when worker admin CLI commands fail#70112

Open
dkranchii wants to merge 1 commit into
apache:mainfrom
dkranchii:fix-edge3-worker-admin-exit-codes
Open

edge3: Exit non-zero when worker admin CLI commands fail#70112
dkranchii wants to merge 1 commit into
apache:mainfrom
dkranchii:fix-edge3-worker-admin-exit-codes

Conversation

@dkranchii

@dkranchii dkranchii commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

add-worker-queues, remove-worker-queues, and set-worker-concurrency raised bare SystemExit after catching a TypeError from the model layer (raised when the target worker is in OFFLINE, OFFLINE_MAINTENANCE, or UNKNOWN state). SystemExit() with no argument yields exit code 0, so shell scripts wrapping these commands saw success and continued as if the queue or concurrency change had been applied, when in fact it had been silently rejected.

Pass the exception message to SystemExit(str(e)) so the process exits with status 1 and writes the failure to stderr. Matches this file's own convention for validation-error paths (e.g. raise SystemExit("Error: No queues specified to add.") a few lines above each fix site) and the raise SystemExit(str(e)) pattern already used in providers/edge3/src/airflow/providers/edge3/cli/worker.py.

The model layer at providers/edge3/src/airflow/providers/edge3/models/edge_worker.py already calls logger.error(error_message) before raising, so the CLI-side logger.error(str(e)) was strictly duplicative and is removed.

Adds parametrized tests over all three subcommands asserting that SystemExit.code carries the error message (i.e. exit status is 1, not 0).

Was generative AI tooling used to co-author this PR?
  • Yes — Cursor

@boring-cyborg boring-cyborg Bot added area:providers provider:edge Edge Executor / Worker (AIP-69) / edge3 labels Jul 20, 2026
@dkranchii
edge3: Exit non-zero when worker admin CLI commands fail
244b190 
The add-worker-queues, remove-worker-queues, and set-worker-concurrency
subcommands raised bare `SystemExit` after catching a TypeError from the
model layer, which yields exit status 0. Shell scripts wrapping these
commands treated failures as success and continued as if the queue or
concurrency change had been applied.

Pass the exception message to SystemExit so the process exits with
status 1 and writes the failure to stderr, matching the file's own
convention for validation-error paths (see the explicit
`raise SystemExit("Error: ...")` calls in the same subcommands) and the
`raise SystemExit(str(e))` pattern already used elsewhere in the edge3
CLI. The model layer already logs the error before raising, so the
redundant logger.error call is removed.
@dkranchii
dkranchii force-pushed the fix-edge3-worker-admin-exit-codes branch from 20d94d1 to 244b190 Compare July 24, 2026 17:33
@dkranchii

dkranchii commented Jul 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@jscheffl @dheerajturaga can you review this.

@dheerajturaga

dheerajturaga commented Jul 24, 2026

Copy link
Copy Markdown
Member

Thanks for tackling this — the exit-code bug is real and the motivation is spot on. I reviewed the change and have one blocker plus a couple of suggestions.

The production fix is correct. Switching the three except TypeError handlers from bare raise SystemExit (exit code 0) to raise SystemExit(str(e)) (exit 1 + message on stderr) is the right call. The model layer already logs the error before raising, so dropping the CLI-side logger.error(str(e)) is a clean de-dup, and raise SystemExit(str(e)) matches the existing convention in cli/worker.py. (Omitting from e is fine here — B904 isn't in the repo's ruff config, and the worker.py sites omit it too.)

🔴 Blocker — the new test fails. The test builds args with types.SimpleNamespace, but these commands are wrapped by @cli_utils.action_cli, whose _check_cli_args (airflow-core/src/airflow/utils/cli.py:56) rejects anything that isn't an argparse.Namespace:

ValueError: 1st positional argument should be argparse.Namespace instance, but is <class 'types.SimpleNamespace'>

That ValueError escapes before any SystemExit, so pytest.raises(SystemExit) never matches and all three cases error out — which is what the red Non-DB tests: providers … edge3 and Provider distributions tests jobs are reporting. Swapping SimpleNamespace → argparse.Namespace (same fields) makes all three pass, and the test then correctly fails without the production change.

🟡 Suggestion — two identical bug sites remain. The same bare-raise SystemExit-after-except TypeError pattern is still present at edge_command.py:329-330 (remote_worker_update_maintenance_comment) and :344-345 (remove_remote_worker); their model functions (change_maintenance_comment, remove_worker) log-and-raise TypeError just like the three you fixed. Folding these two into this PR (and into the parametrized test) would make the fix complete rather than partial.

Minor — test comment. The comment says it checks "exit code 1," but the assertion checks that .code equals the message string; the exit-status-1 behavior happens later when the interpreter prints a non-int code. Might reword to avoid conflating the two.

Drafted-by: Claude Code (Opus 4.8); reviewed by @dheerajturaga before posting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jscheffl jscheffl Awaiting requested review from jscheffl jscheffl is a code owner
@dheerajturaga dheerajturaga Awaiting requested review from dheerajturaga dheerajturaga is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

area:providers provider:edge Edge Executor / Worker (AIP-69) / edge3

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dkranchii @dheerajturaga